fix: make AT128 scans align with scan_phase #84
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Plot angle timings rel to ToS
mojomex
force-pushed
the
at128_top_of_second_diag
branch
from
54cb3b7 to
e715687
Compare
mojomex
commented
Nov 1, 2023
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
=========================================
+ Coverage 8.28% 13.83% +5.54%
=========================================
Files 87 45 -42
Lines 8456 4526 -3930
Branches 852 799 -53
=========================================
- Hits 701 626 -75
+ Misses 7180 3346 -3834
+ Partials 575 554 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@mojomex |
* Scans are always cut at the start of a mirror now * Scan phase is sent as-is (in output coordinates) to the sensor, since it handles the conversion internally now.
drwnz
requested changes
Nov 29, 2023
mojomex
force-pushed
the
at128_top_of_second_diag
branch
from
14b43c0 to
a0b14d4
Compare
|
Done 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Related Links
TIER IV INTERNAL LINK -- AT128
scan_angletroubleshooting logDescription
Before this PR, AT128's pointcloud timestamps were not aligned to top of second at the set
scan_angle.The refactoring in #42 introduced a wrong calculation for scan cutting in AT128 (subtracted
scan_anglein output coordinates from the raw azimuth value).Further, AT128's 3.50.6 software version set
scan_angleas-is, without converting internally to encoder angles first.This has been fixed in 3.50.8.
This PR fixes this issue via the following code changes:
scan_anglein the respectiveangle_corrector_...implementations, not inhesai_decoderscan_phaseparameter in the AT128 configuration file to30.0(the lowest legal value)Review Procedure
Run Nebula with AT128, set
scan_angleto different values30 <= scan_angle <= 150and observe the timestamps of the output pointclouds:ros2 topic echo --field header.stamp.nanosec /pandar_pointsIf the second and third digits are always close to
0, the fix is working.Remarks
Because channel corrections can range from around -3 to +3 deg for the same encoder angle, there is a range of output angles associated with the encoder angle. Currently, the scan cutting/angle conversion code ignores this. It is a design decision whether to align to the earliest channel to hit
scan_angleor to align channel-agnostically (without factoring in correction). The latter ends up roughly in the mean of all channels.Here is an excerpt from the extensive analysis, showing
scan_angle = 90 deg. Black is aligned to0.X000... s, yellow is maximally unaligned (0.Y999... s). Channel 0 is the innermost ring, channel 127 the outermost. One can see that the points at the same output angle are not all aligned to ToS.Pre-Review Checklist for the PR Author
PR Author should check the checkboxes below when creating the PR.
Checklist for the PR Reviewer
Reviewers should check the checkboxes below before approval.
Post-Review Checklist for the PR Author
PR Author should check the checkboxes below before merging.
CI Checks